feat: [FC-0044] XBlock's children API as DRF - #34055
Conversation
|
Thanks for the pull request, @ruzniaievdm! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
Hi @ruzniaievdm! Just flagging that there's a couple failing checks. Would you mind taking a look? Thanks! |
36c9991 to
417187a
Compare
417187a to
3543267
Compare
|
Sandbox deployment successful. Sandbox LMS is available at https://pr-34055-ff10e3.sandboxes.opencraft.hosting Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-34055-ff10e3-51564656-6173868163-tutor-config.yml |
|
Sandbox deployment successful. Sandbox LMS is available at https://pr-34055-ff10e3.sandboxes.opencraft.hosting Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-34055-ff10e3-51564656-6174127840-tutor-config.yml |
arbrandes
left a comment
There was a problem hiding this comment.
I have tested this in the sandbox and locally as described, and cannot find any problems.
I think a second opinion is warranted on how to solve the split_test bug. (Plus, I'm technically not a CC here, so can't give final approval.)
There was a problem hiding this comment.
This seems difficult to maintain, but I can't think of a cheaper workaround. The question is if there's a more architecturally sound way of doing it, and if we want to hold off on merging this until we find one.
@kdmccormick or @feanil, thoughts?
There was a problem hiding this comment.
@arbrandes Agreed, we should not hard code block types if at all possible. There are places that do it already, but I would rather avoid adding more.
The comment is somewhat confusing to me. What is the meaning of an item being in this list? Is it that is type should be ignored when determining the icon?
3543267 to
6f45f44
Compare
* fix: wrap into try/except block getting icon for xblock * fix: revision after review
6f45f44 to
8979134
Compare
|
Sandbox deployment successful 🚀 |
|
@arbrandes I apologize for confusing you, indeed, in the previous version namely up to and including the second commit, you did not see the error, there was a workaround to this problem. And if you had made these changes, you would have seen the problem.
@arbrandes @kdmccormick After reviewing your comments we decided to take a different approach to solving this problem. Please review again. |
arbrandes
left a comment
There was a problem hiding this comment.
I have no objections, this time around.
| log.warning( | ||
| "Error while getting block instance for descriptor with location: [%s]", | ||
| self.location | ||
| ) |
There was a problem hiding this comment.
Ok, this feels much more manageable. 👍🏼
|
@ruzniaievdm 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Settings
Description
This PR addresses a need in the development of the Course Unit Page feature within the frontend-app-course-authoring repository. The core objective is the implement of a DRF endpoint designed to render and deliver children data for container xblock.
This PR also includes:
create_xblock_infohandler. It needs to display type of unit icons for the unit course page. Each unit must have one of the following types:video,vertical,problem,lock.Testing instructions
make dev.up.lms+cmsand make checkout on this branch.http://localhost:18010/api-docs./api/contentstore/v1/container/vertical/{usage_key_string}/children.http://localhost:18010/container/{usage_key_string}and make sure nothing has changed.usage_key_stringfrom previous step and paste it to required API endpoint to get data for unit.How to reproduce an error
The problem arose at the stage of defining icons for children xblock's of the unit.
Just when we tried to get
get_icon()for xblock which havesplit_testchildren.